PowerVS: Add an only IPI regions list #10
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hamzy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ppc64le-cloud-bot
added
the
size/L
|
@mkumatag How about this version? |
In order for the IPI installer to be able to use this module, it needs the ability to have the list of regions to only include supported regions.
hamzy
force-pushed
the
PowerVS-Only-support-PER2
branch
from
800109f to
bbd3d40
Compare
There was a problem hiding this comment.
I'm little uncomfortable overriding the variables like Regions, wondering if there any better way to deal with this? may be a better implementation of this utility itself! 🤔
Any idea will be appreciated @Karthik-K-N @dharaneeshvrd
| } | ||
|
|
||
| // Regions provides a mapping between Power VS and IBM Cloud VPC and IBM COS regions. | ||
| var Regions = map[string]Region{ | ||
| var Regions = FullRegions |
There was a problem hiding this comment.
Why do we need to do like this? Any reason behind this?
IMHO, we can use the existing map itself with the addition of IPISupported field on each region.
| }, | ||
| } | ||
|
|
||
| var IPIRegions = map[string]Region{} |
There was a problem hiding this comment.
It is used. Have you searched the code?
| var Regions = FullRegions | ||
|
|
||
| // Switch the list of regions to all of the regions. | ||
| func UseFullRegions () { |
There was a problem hiding this comment.
Not required IMO, if we just modify the existing Regions map itself.
| } | ||
|
|
||
| // Switch the list of regions to only the regions supported by the IPI installer. | ||
| func UseIPIRegions () { |
There was a problem hiding this comment.
| func UseIPIRegions () { | |
| func GetIPIRegions () map[string]Region { |
This can be used to filter the IPI regions alone and return it.
| } | ||
| } | ||
|
|
||
| Regions = IPIRegions |
There was a problem hiding this comment.
| Regions = IPIRegions | |
| return IPIRegions |
|
@dharaneeshvrd The reason why I do it this way is so the user can switch between the two lists at any time. @mkumatag The other way I see to implement this is to alter every API call to add an additional parameter to indicate that only IPI is requested. This would require everyone to change and therefore be unacceptable. Feel free to submit a PR for code you don't have a problem with. It seems to me that this is a situation where the approver does not work on the IPI project and therefore does not feel the constant pain of having to implement new zones and/or regions. |
|
Does adding the parameter really mean we'll need to expose it in every API call? Another option is a function that returns a list of keys into the Region list (dal, wdc, mad, etc) |
|
AFAIK Go does not have a default parameter. I don't even think you can have two different functions with the same name but different number of parameters. |
|
@hamzy I understood what you are trying to do now. The changes will work. Please see below approach will work! Another suggestion is if IPI is the major consumer of this utility pkg, then we can keep that env as default to IPIRegions and other consumers like hypershift can keep that env var to false and use it. |
|
@dharaneeshvrd So you would rather change 11 functions, and all future functions, than one list? Would you agree that this way is more maximally invasive than the previous way? |
|
Agree, it's a lot of change. I think current implementation looks good to me! |
In order for the IPI installer to be able to use this module, it needs the ability to have the list of regions to only include supported regions.